Repository navigation
backport: v0.26 bitcoin#24148 - #56
DCG-Claude wants to merge 1 commit into
Conversation
7485a01 to
edd20c1
Compare
Dash's src/.clang-tidy enforces performance-no-automatic-move as a hard error (upstream did not at the time of bitcoin#24148), so the 🤖 backportsys, on behalf of the Dash backport pipeline. |
The attached log contains only container/network teardown lines from the runner — no compiler diagnostic, no failing unit or functional test, and no TSan data-race report — so there is nothing in it that points at this branch. bitcoin#24148 touches only descriptor/miniscript parsing plus tests (the new wallet_miniscript.py is a watch-only import test and the framework here already provides add_wallet_options and skip_if_no_sqlite), and adds no threading or concurrency code that a TSan job could legitimately trip over. I read this as a job/infrastructure flake; please rerun linux64_tsan-test, and if it fails again please attach the portion of the log with the actual error. 🤖 backportsys, on behalf of the Dash backport pipeline. |
|
This pull request has conflicts, please rebase. |
ffc79b8 qa: functional test Miniscript watchonly support (Antoine Poinsot) bfb0367 Miniscript support in output descriptors (Antoine Poinsot) 4a08288 qa: better error reporting on descriptor parsing error (Antoine Poinsot) d25d58b miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot) c38c7c5 miniscript: don't check for top level validity at parsing time (Antoine Poinsot) Pull request description: This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of bitcoin#24147 for a description of Miniscript and a rationale of having it in Bitcoin Core. On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support. A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at bitcoin-core/qa-assets#92. The Miniscript descriptors used in the unit tests here and in bitcoin#24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript. This PR contains code and insights from Pieter Wuille. ACKs for top commit: Sjors: re-utACK ffc79b8 achow101: ACK ffc79b8 w0xlt: reACK bitcoin@ffc79b8 Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9 Dash adaptations: - src/script/descriptor.cpp: Dash has no segwit, so `ParseScriptContext::P2WSH` does not exist. Miniscript is wired to Dash's only hash-wrapped-script context, `ParseScriptContext::P2SH`, at all five upstream P2WSH sites: `KeyParser::FromString`, `KeyParser::FromPKBytes`, `KeyParser::FromPKHBytes`, the miniscript gate in `ParseScript`, and the miniscript gate in `InferScript`. Miniscript expressions therefore live inside `sh()` instead of `wsh()`. - src/script/descriptor.cpp: the user-visible error string was written as "Miniscript expressions can only be used in sh" instead of upstream's "... can only be used in wsh". - src/script/descriptor.cpp: upstream's new `InferPubkey` body uses `ConstPubkeyProvider(0, pubkey, /*xonly=*/false)` and a 3-arg `OriginPubkeyProvider`. Dash's `ConstPubkeyProvider` takes 2 args (no xonly flag) and `OriginPubkeyProvider` takes a trailing `apostrophe` flag, so the moved-up `InferPubkey` was written as `ConstPubkeyProvider(0, pubkey)` / `OriginPubkeyProvider(0, std::move(info), std::move(key_provider), /*apostrophe=*/false)`. - src/script/descriptor.cpp: `InferXOnlyPubkey` and upstream's `InferMultiA` were dropped rather than added — Dash has no `XOnlyPubKey` inference, no `MatchMultiA`, and no `MultiADescriptor`. - src/script/descriptor.cpp: the `DescriptorImpl` conflict was resolved keeping Dash's existing comment wording on `m_pubkey_args` while taking upstream's move of the `protected:` label above that member (so `MiniscriptDescriptor` can reach it). - src/test/descriptor_tests.cpp: the DoCheck hunks were resolved onto Dash's `Parse(prv, keys_priv, error)` / `UseHInsteadOfApostrophe(pub)` flow, keeping Dash's apostrophe-vs-h handling and taking upstream's `BOOST_CHECK_MESSAGE(EqualDescriptor(...))` upgrades. - src/test/descriptor_tests.cpp: upstream's negative Miniscript tests were rewritten from `wsh(...)` to `sh(...)`, and the expected error for the context check is "Miniscript expressions can only be used in sh". Upstream's private descriptors use Bitcoin mainnet WIF keys, which do not decode under Dash's base58 prefixes and could not be re-encoded in this environment, so the hex public keys are used for both the private and the public descriptor in those `CheckUnparsable` calls; a comment in the file records this. - test/functional/wallet_miniscript.py: every descriptor is wrapped in `sh(...)` instead of `wsh(...)` (the four `MINISCRIPTS` policies via `descsum_create(f"sh({ms})")`, and the insane-descriptor sanity check). All four policies compile to redeemScripts well under the 520-byte `MAX_SCRIPT_ELEMENT_SIZE` push limit. - test/functional/wallet_miniscript.py: added `add_options` calling `self.add_wallet_options(parser, legacy=False)`. Dash's test framework requires this declaration, otherwise `self.options.descriptors` is `None` and the node is started with `-disablewallet`. - test/functional/test_runner.py: the new entry is `'wallet_miniscript.py --descriptors'` — the explicit flag is needed because Dash's framework runs with `REQUIRE_WALLET_TYPE_SET`. Upstream's hunk also carried the context line `'feature_maxtipage.py'`; it was dropped because Dash already lists that test earlier in the file and re-adding it would duplicate the entry. Not applicable to Dash (intentionally omitted): - src/test/descriptor_tests.cpp: Two positive Miniscript Check() cases (wsh(...) and sh(wsh(...))). They expect segwit scriptPubKeys (0020..., P2SH-P2WSH), OutputType::BECH32/P2SH_SEGWIT and MIXED_PUBKEYS, none of which exist in Dash. The positive sh() path is covered by wallet_miniscript.py, which passes. (reviewer: not for Dash) - src/test/descriptor_tests.cpp: tr(miniscript) CheckUnparsable case and the long tr() fuzz-regression CheckUnparsable case. Dash has no taproot (no tr() descriptor, no P2TR context). (reviewer: not for Dash) - src/test/descriptor_tests.cpp: raw(miniscript) CheckUnparsable case whose public side is sh(miniscript) expecting 'can only be used in wsh'. In Dash sh(miniscript) is the valid context, so the pairing cannot carry over. The same error path is tested by the top-level (no wrapper) CheckUnparsable case. (reviewer: not for Dash) - src/test/descriptor_tests.cpp: 'Invalid checksum' CheckUnparsable case on a wsh(miniscript)#abcdef12 descriptor. Its expected computed checksum is for the wsh() string. This omission was carried over unchanged from the already-reviewed backport. It only exercises the generic checksum error path, which Dash's existing checksum tests already cover. No prerequisite blocks it, so an sh() variant could be added later as a test-only follow-up. (reviewer: not for Dash) Replayed onto a newer base. Dash adaptations: - src/script/descriptor.cpp: InferPubkey is moved above KeyParser as upstream 24148 does, but the moved body is develop's current one: it keeps the IsValidNonHybrid() and TOP/P2SH-only-uncompressed checks and the named `ctx` parameter that the partial bitcoin#28602 backport (13def7a) added - src/script/descriptor.cpp: KeyParser::FromPKBytes/FromPKHBytes check InferPubkey's nullable result (`if (auto pubkey_provider = InferPubkey(...))`), which is how upstream bitcoin#28602 rewrote these KeyParser methods (it landed after 24148). Develop's InferPubkey can now return nullptr, and leaving these unchanged would push null providers. This replaces the prior diff's `pubkey.IsValid()` check in FromPKBytes, because InferPubkey's IsValidNonHybrid() check covers it - src/script/descriptor.cpp: InferScript puts the P2SH Miniscript inference block before the 'top-level only descriptors' early return that bitcoin#28067 added (48a3b6c). This is the same order as upstream master (see 744157e), so sh(miniscript) inference is not cut off - src/script/miniscript.h: the Parse() conflict is resolved to upstream's `return std::move(constructed.front());`. Develop had already made tl_node non-const via bitcoin#26707, so the prior diff's DecodeScript `const NodeRef` -> `NodeRef` hunk is already on develop and is no longer in this diff - src/test/descriptor_tests.cpp: the Miniscript CheckUnparsable cases from the prior diff (sh() context, hex pubkeys instead of Bitcoin mainnet WIF keys) are appended after develop's CheckInferDescriptor cases from bitcoin#28602; both sets are kept - Carried over unchanged from the prior reviewed backport: Miniscript is only allowed inside sh() (Dash has no segwit), so the error text is 'Miniscript expressions can only be used in sh', and KeyParser and InferScript use ParseScriptContext::P2SH instead of P2WSH; wallet_miniscript.py wraps each miniscript in sh() instead of wsh() Not applicable to Dash (intentionally omitted): - src/test/descriptor_tests.cpp: Upstream's two positive Miniscript Check() cases are wsh()/sh(wsh()) descriptors with witness-program scripts and Bitcoin mainnet WIF keys. Dash has no segwit. This omission is carried over from the prior reviewed backport and is documented in a code comment; wallet_miniscript.py covers the sh() positive path
edd20c1 to
90ae269
Compare
|
every backport on this branch is either on develop already or could not be carried onto the current develop; nothing left to carry - bitcoin#24148: could not be fixed: Confirmed: upstream relies on P2WSH limits inside IsValid()/CheckStackSize() (script size, stack items), which Dash's bitcoin#24147 backport dropped, and moving |
Automated Bitcoin Core v0.26 backports, batch
backport-0.26-b033-src.90ae2694b4Provenance
Each commit passed: cherry-pick (adapted by an Opus lane only where conflicts existed), build, touched tests, a mechanical diff-of-diffs check (every upstream hunk landed; no added line without an upstream counterpart), and an independent Opus verification lane where anything was adapted. Gate rows and lane artifacts are in the backportsys DB.